Skip to content

PyPi Wheel Generation#3794

Draft
amd-juwillia wants to merge 13 commits intodevelopfrom
juwillia/python-final
Draft

PyPi Wheel Generation#3794
amd-juwillia wants to merge 13 commits intodevelopfrom
juwillia/python-final

Conversation

@amd-juwillia
Copy link
Collaborator

No description provided.

@github-actions github-actions bot added project: amdsmi github actions Pull requests that update GitHub Actions code labels Mar 5, 2026
@amd-juwillia amd-juwillia force-pushed the juwillia/python-final branch from 859f2ef to 57593b8 Compare March 5, 2026 17:26
@marifamd marifamd force-pushed the juwillia/python-final branch from 57593b8 to b4a0d5a Compare March 5, 2026 18:15
@amd-juwillia amd-juwillia force-pushed the juwillia/python-final branch from b4a0d5a to 4d9a212 Compare March 5, 2026 18:33
Signed-off-by: Justin Williams <Justin.Williams@amd.com>
@marifamd marifamd force-pushed the juwillia/python-final branch from b549d6e to 9639199 Compare March 5, 2026 19:43
Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
@marifamd marifamd force-pushed the juwillia/python-final branch 2 times, most recently from bef95b3 to 5618d07 Compare March 6, 2026 15:54
- Link amdsminic static library into libamd_smi_python.so to resolve undefined NIC symbols
- Prevent pip-context wrapper from falling back to system libamd_smi.so to avoid dual-loading segfault
- Add trailing newline to setup.cfg.in
@amd-juwillia amd-juwillia force-pushed the juwillia/python-final branch from 5618d07 to 87e9051 Compare March 6, 2026 20:24
Signed-off-by: Justin Williams <Justin.Williams@amd.com>
Signed-off-by: Justin Williams <Justin.Williams@amd.com>
@amd-juwillia amd-juwillia force-pushed the juwillia/python-final branch from 68aa824 to b0dc79f Compare March 8, 2026 03:12
except Exception:
_log_version_and_path_diagnostics()
print("[amdsmi-cli] Failed to read amdsmi Python package version; aborting to avoid incompatibility.")
sys.exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where my code is failing to run in the container. Once we get this fixed I will approve as the rest looks great

class struct_amdsmi_pcie_info_t(Structure):
pass

class struct_pcie_static_(Structure):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to this PR, did you have a stray commit in here? I think we'd want to have this separate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Signed-off-by: Justin Williams <Justin.Williams@amd.com>
@amd-juwillia amd-juwillia force-pushed the juwillia/python-final branch from 00aaf6a to 83dea0d Compare March 13, 2026 18:22
Signed-off-by: Justin Williams <Justin.Williams@amd.com>
Signed-off-by: Justin Williams <Justin.Williams@amd.com>
Copy link
Member

@marbre marbre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a brief look myself and furthermore let an agent scan through this. As pointed out earlier this will need a written-down RFC before going further with the implementation. Here are some of the comments my agent left:


Python Wheel Version Tagging

The package has no C extension modules — no ext_modules, no cython, no cffi. libamd_smi_python.so is a C++ shared library bundled as package_data, not as a Python extension. pyproject.toml.in and setup.cfg.in both confirm this. Setuptools builds packages with no C extensions as py3-none-linux_x86_64 — Python version agnostic.

The manylinux workflow loops over six interpreters (cp38–cp313), but because the wheel tag is py3-none-linux_x86_64 for all of them, each iteration produces a file with the same name. Successive builds overwrite each other in $RAW_WHEELS. The "six wheels" outcome advertised by the workflow is not what actually happens.

auditwheel was designed for ABI-tagged wheels (cpXX-cpXX-linux_x86_64). Its behavior on py3-none-linux_x86_64 wheels that bundle .so data files is not well-defined and may produce incorrect tags, refuse the input, or pass it through unchanged.

Metadata Duplication:

Both files setup.cfg.in and pyproject.toml.in exist on the branch and both declare the full package metadata (name, version, author, classifiers, etc.). With setuptools ≥ 61, pyproject.toml takes precedence, but having two authoritative metadata sources creates a maintenance hazard: a version bump that updates one but not the other silently produces stale metadata.

CI / Workflow

  • rpm-test now needs: [manylinux-build, debian-test]. This creates a cross-platform serialization: RPM tests cannot start until all Debian matrix legs complete (Ubuntu20, Ubuntu22, Ubuntu24, Debian10). Previously the RPM build was independent. With continue-on-error: true on Debian legs this may not block execution, but it increases total wall time.

  • Manylinux gate hardcodes /opt/python/cp38-cp38/bin/python3. If manylinux_2_28 drops cp38 support in a future image update, the gate permanently breaks.

  • chmod 777 "$WHEEL_OUT" in manylinux-build.yml is overly broad for a container build artifact directory.

done


- name: Verify Wheel in Site-Packages
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for not moving this to a dedicated scripts?

echo "Displaying Unit Test Results for ${{ matrix.os }}"
cat /tmp/test-results-${{ matrix.os }}/unit_test_output.txt || echo "No unit test results found for ${{ matrix.os }}"

- name: Perf Test Results
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why cat perf test results here instead of uploading them to a more accessible location?

class struct_amdsmi_pcie_info_t(Structure):
pass

class struct_pcie_static_(Structure):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

@marbre
Copy link
Member

marbre commented Mar 16, 2026

As an additional note, I don't quite follow why you try to leverage .pth files which should be limited to a minimum:

https://docs.python.org/3/library/site.html

Note An executable line in a .pth file is run at every Python startup, regardless of whether a particular module is actually going to be used. Its impact should thus be kept to a minimum. The primary intended purpose of executable lines is to make the corresponding module(s) importable (load 3rd-party import hooks, adjust PATH etc). Any other initialization is supposed to be done upon a module’s actual import, if and when it happens. Limiting a code chunk to a single line is a deliberate measure to discourage putting anything more complex here.

Rather see https://www.debian.org/doc/packaging-manuals/python-policy/#module-path. Debian use this even though it is not standard, check how RPM-based distros handle it. As above, this needs a written down plan.

Signed-off-by: Justin Williams <Justin.Williams@amd.com>
@amd-juwillia amd-juwillia force-pushed the juwillia/python-final branch from 379bccb to 6d00c1a Compare March 19, 2026 14:20
Signed-off-by: Justin Williams <Justin.Williams@amd.com>
@amd-juwillia amd-juwillia force-pushed the juwillia/python-final branch from 0f80d99 to 4b1c266 Compare March 21, 2026 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

github actions Pull requests that update GitHub Actions code organization: ROCm project: amdsmi

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants